-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Empty prefix support #183
Empty prefix support #183
Conversation
This fixes the issue, tested with a JSON output and a Turtle output (the latter really shows the difference and correctness). If any more testing needs to be done before this pull request can happen I'd be glad to look if I can be of assistance.
@coreation can you add corresponding unit-tests, please? |
Ofcourse, this test should basically add an empty prefix, and check (when a triple is added with an empty prefix) if it is lited in the @Prefix turtle output? Anything else I should add to it? |
Yup.
|
Ok, I'll run the tests before I add mine, to assess how many test normally fail ;). |
@indeyets I don't seem to get a unittest working, I installed it through composer (and updated), yet if I run any test I get: PHP Warning: require_once(PHPUnit/Framework/IncompleteTestError.php): failed to open stream: No such file or directory in /path/to/easyrdf/test/TestHelper.php on line 41 If I look at my autoload classmap file, I see the class reference (be it with _ instead of folder separation but that shouldn't form a problem?) I run php 5.4. |
@coreation are you sure you're on "devel" branch? it looks like you have phpunit4 installed. That can happen on "master", but devel is fixed to require phpunit3 |
Well I'm on the "patch-1 branch" since I didn't for the repository myself. Looking at the network, I see that the patch-1 branch has been forked from the devel branch. Just to be sure I checked out the devel branch, composer update didn't update any dependencies and this: php -v test/examples/BasicTest.php still gives this error: PHP Warning: require_once(PHPUnit/Framework/IncompleteTestError.php): failed to open stream: No such file or directory in /path/to/easyrdf/test/TestHelper.php on line 41 |
@coreation ok. that's not how you run tests. full test suite: just the tests you should care about: |
Ah, sorry I'm used to having a phpunit.xml file which does that for me ;). |
@indeyets Ok, so I did the following:
If there's anything more I should contribute on this issue let me know. |
@coreation Can you make related turtle-serializer test? that's an interesting part. current test doesn't create any document, so I'm not sure what you tested against external validator. |
$url | ||
); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 lines above are not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean this function can be removed from the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope. I mean that 2 empty lines are not needed :)
So in the Turtle Serializer test I should add something in the same line as this function: public function testSerialise()
{
$joe = $this->graph->resource(
'http://example.com/joe#me',
'foaf:Person'
);
$joe->set('foaf:name', 'Joe Bloggs');
$joe->set(
'foaf:homepage',
$this->graph->resource('http://example.com/joe/')
);
$turtle = $this->serialiser->serialise($this->graph, 'turtle');
$this->assertSame(
"@prefix foaf: <http://xmlns.com/foaf/0.1/> .\n".
"\n".
"<http://example.com/joe#me>\n".
" a foaf:Person ;\n".
" foaf:name \"Joe Bloggs\" ;\n".
" foaf:homepage <http://example.com/joe/> .\n\n",
$turtle
);
} But then for an empty namespace prefix? |
@indeyets provided the extra unittest in the Turtle seriliazer, and removed the 2 white lines. |
@coreation thank you. l hope to check/merge it later today. I just understood, that we also need to validate that it doesn't break other serialisers |
@indeyets No problem, so other serialisers might break because you can add an empty namespace? Makes sense, since that premisse was never present so far. Keep me posted. |
@indeyets Do you need me to write a serialise test for the other serialisers with an empty prefix as well? Or is that taken care of? |
@coreation if you have some time — that would be cool. I just can't fine a minute myself :-( |
I'll look into it the coming days. On Wed, Apr 23, 2014 at 10:10 AM, Alexey Zakhlestin <
Met vriendelijke groeten, Jan Vansteenlandt |
Added the empty namespace test in most of the serializer tests. The ones I didn't edit, were the ones I think that don't need extra testing. Could be wrong ofcourse. |
@coreation I modified your RDF/XML test a bit and we clearly have a breakage. diff --git a/test/EasyRdf/Serialiser/RdfXmlTest.php b/test/EasyRdf/Serialiser/RdfXmlTest.php
index 2f7f228..7464795 100644
--- a/test/EasyRdf/Serialiser/RdfXmlTest.php
+++ b/test/EasyRdf/Serialiser/RdfXmlTest.php
@@ -426,6 +426,7 @@ class EasyRdf_Serialiser_RdfXmlTest extends EasyRdf_TestCase
'foaf:homepage',
$this->graph->resource('http://example.com/joe/')
);
+ $joe->set('http://foo/bar/test', 'test');
$rdf = $this->serialiser->serialise($this->graph, 'rdfxml'); produces this invalid document: <?xml version="1.0" encoding="utf-8" ?>
<rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
xmlns:foaf="http://xmlns.com/foaf/0.1/"
xmlns:="http://foo/bar/">
<foaf:Person rdf:about="http://foo/bar/me">
<foaf:name>Joe Bloggs</foaf:name>
<foaf:homepage rdf:resource="http://example.com/joe/"/>
<:test>test</:test>
</foaf:Person>
</rdf:RDF> |
Okay, that clearly breaks it indeed. If you need anything from me, let me know. |
Any update to be fetched on this? :) |
nudge? |
merged |
This fixes issue #181, tested with a JSON output and a Turtle output (the latter really shows the difference and correctness). If any more testing needs to be done before this pull request can happen I'd be glad to look if I can be of assistance.